Fix FPs in bitwiseOnBoolean#3455
Conversation
|
Actually, thinking about it more. I think we can warn for |
| " if(a | b) {}\n" | ||
| "}"); | ||
| ASSERT_EQUALS( | ||
| "[test.cpp:2]: (style, inconclusive) Bitwise expression 'a|b' with boolean expression 'a' is equivalent to 'b'?\n", |
There was a problem hiding this comment.
I don't understand this. maybe a is true and b is 0.
There was a problem hiding this comment.
Ah thats right, then let me remove the warning when using a|b.
|
Anymore feedback? |
danmar
left a comment
danmar
left a comment
There was a problem hiding this comment.
I don't feel I understand why we write these warnings. So your idea is that if lhs (or both lhs and rhs?) are const expressions then we should warn? Is the motivation for that to prevent bugs or to speedup?
In the true positive test cases the expressions are actually conditions. I can actually feel that the code if (a || b) does look better than if (a | b). For purely stylistic reasons. But this checker does not check if the expression is a condition. Should it?
| continue; | ||
| if (!isConstExpression(tok->astOperand1(), mSettings->library, true, mTokenizer->isCPP())) | ||
| continue; | ||
| if (!isConstExpression(tok->astOperand1(), mSettings->library, true, mTokenizer->isCPP())) |
There was a problem hiding this comment.
did you intend to pass astOperand2 here?
There was a problem hiding this comment.
Yep, good catch. I am surprised that duplicateCondition didnt catch this.
It is to improve clarity, which can help prevent bugs. First, if we are going to suggest to use logical operators instead of bitwise operators, then we need to make sure this suggestion will do what the user intended. So if Secondly, by making the intention of using On the flip side we could consider suggesting bitwise operators when there is side effects, but I suspect there may be too many FPs from this as using logical operators can be intentional to avoid running a function with side effects based on previous conditions. We can replace
For |
ok good. I agree we need to check for side effects.
I do like to use |
…ck into fp-bitwise-bool-non-const
I dont really see the difference with assign to a bool or using in an if statement. It can still provide clarity using logical ops that there are no side effects. |
|
ok I guess this is a improvement at least however I still have the feeling it's too controversial. |
2 participants
We no longer warn on cases where logical operators are not equivalent to the bitwise version:
|with||when one operand is an integer produce different results so we shouldn't warnI kept it inconclusive, but I think we can remove the inconclusive now. We could make the
|with integer inconclusive because even though it isnt equivalent it could still be suspect(but maybe this is better in an addon).